@claude Review once

PR Review

Great overall design — the four-level precedence chain, setDomains(), and load_workflows tool are all clean and well-tested. A few things to address:

🔴 Must fix: Default fallback should remain code, not empty Set

The final fallback in parseEnabledDomains() was changed from new Set(['code']) to new Set(). This changes the default behavior — users who set no config now get all domains loaded instead of just code.

The goal of this PR is to make the default configurable and switchable, not to change what the default is. code should remain the out-of-the-box default.

Fix:

// packages/core/src/workflow-manager.ts
// Change this:
logger.debug('No domain configuration found, loading all workflows');
return new Set();

// To this:
logger.debug('No domain configuration found, using default: code');
return new Set(['code']);

This also likely fixes the CI failure — the game-beginner.yaml validation test now runs because children domain loads by default (empty Set = all domains). Restoring Set(['code']) would exclude children from the default load and likely make that test pass again.

The affected tests in workflow-domain-filtering.test.ts and workflow-domains-precedence.test.ts that assert "all workflows load by default" should be reverted to assert code-only default.

⚠️ Should fix: getActiveWorkflow() dead code guard

The guard in setDomains() can never trigger since getActiveWorkflow() always returns null:

const activeWorkflow = this.getActiveWorkflow(); // always null
if (activeWorkflow && ...) { // dead branch — never executes

This silently promises safety that isn't delivered. Please either remove the guard and add a // TODO comment, or document clearly that the guard is inactive:

// NOTE: Active workflow conflict detection is not yet implemented.
// getActiveWorkflow() always returns null until ConversationManager integration is complete.
// Callers can switch domains freely regardless of active workflow state.

⚠️ Should fix: setDomains('') behavior should be documented

setDomains('') produces an empty Set which loads all workflows — but callers might expect it to reset to the default (code). This is especially surprising after restoring Set(['code']) as the default. Please document this in the JSDoc:

/**
 * @param domains - Comma-separated string or array of domain names.
 *   Pass an empty string or empty array to load ALL workflows (no domain filtering).
 *   To reset to the default domain, pass 'code' explicitly.
 */

💡 Nice to have: Use constructor param in getAllAvailableWorkflows() to avoid env var mutation

The current implementation temporarily mutates process.env['WORKFLOW_DOMAINS'], which is not safe under concurrent calls. Since defaultDomains constructor param was added in this PR, use it instead:

public getAllAvailableWorkflows(): WorkflowInfo[] {
  const allDomains = process.env['DEFAULT_ALL_DOMAINS'] ||
    'code,architecture,office,sdd,sdd-crowd,skilled,children';
  const tempManager = new WorkflowManager({ defaultDomains: allDomains });
  return tempManager.getAvailableWorkflows();
}

No env var mutation, no try/finally needed, safe for concurrent use.

💡 Nice to have: Deduplicate KNOWN_DOMAINS list

opencode-plugin/src/tool-handlers/load-workflows.ts duplicates the domain list from DOMAIN_DESCRIPTIONS in core. If a new domain is added to core, the plugin must be manually updated. Consider exporting a KNOWN_DOMAIN_NAMES constant from core and importing it in the plugin.

Summary: The architecture and test coverage are solid. The one must-fix is the default fallback — one line change that likely heals the CI failure too. Everything else is polish.